Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wpimath] Add affine transformation constructors and getters to geometry API #7430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

calcmogul
Copy link
Member

Fixes #7429.

@calcmogul calcmogul requested a review from a team as a code owner November 24, 2024 06:29
@calcmogul calcmogul force-pushed the wpimath-add-affine-transformation-constructors-and-getters-to-geometry-api branch from 60a319b to 4d2c89e Compare November 24, 2024 06:33
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll note that Pose2d.toMatrix() is not a direct substitute for the StateSpaceUtil functions since those functions return vectors while toMatrix() returns an affine transformation matrix. I'm still fine with deprecating, but maybe we should change the suggestion since going from the matrix to the vector is at least as complicated as just making the vector yourself?

@calcmogul calcmogul force-pushed the wpimath-add-affine-transformation-constructors-and-getters-to-geometry-api branch 2 times, most recently from 6875010 to 7f2049b Compare November 25, 2024 17:10
@calcmogul
Copy link
Member Author

calcmogul commented Nov 25, 2024

poseTo3dVector() doesn't save much typing

public static Matrix<N3, N1> poseTo3dVector(Pose2d pose) {
  return VecBuilder.fill(
    pose.getTranslation().getX(),
    pose.getTranslation().getY(),
    pose.getRotation().getRadians()
  );
}

for a class that already has a lot of moving parts:

public void addVisionMeasurement(Pose2d visionRobotPoseMeters, double timestampSeconds) {
  m_latencyCompensator.applyPastGlobalMeasurement(
          Nat.N3(),
          m_observer, m_nominalDt,
          StateSpaceUtil.poseTo3dVector(visionRobotPoseMeters),
          m_visionCorrect,
          timestampSeconds
  );
}

I don't see a use for these functions outside this very specific instance. They aren't even useful for affine transformations directly, as you pointed out.

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, for what it's worth.

@calcmogul calcmogul force-pushed the wpimath-add-affine-transformation-constructors-and-getters-to-geometry-api branch from 3288b3c to 7e1dc0c Compare November 25, 2024 17:46
@calcmogul
Copy link
Member Author

calcmogul commented Nov 25, 2024

Unfortunately, we can't merge it until we drop GCC 10, cuz we hit a Raspberry Pi compiler ICE in unrelated code.
https://github.com/wpilibsuite/allwpilib/actions/runs/12015767540/job/33494553453?pr=7430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add affine transformation constructors and getters to geometry API
2 participants